Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user script setting #2836

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add user script setting #2836

wants to merge 2 commits into from

Conversation

milhnl
Copy link

@milhnl milhnl commented Sep 5, 2024

This PR adds the Custom JS option.

I tried to follow the "Custom JS" translations, and have been running this for myself for about a week now.

I know I am not good at testing software. I don't really know what could break, I tried adding a script, and removing it, but this isn't a feature with a lot of varying code paths. Feedback is very welcome! I don't really have a lot of experience with Go.

Fixes #1742


Do you follow the guidelines?

Copy link
Member

@fguillot fguillot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are few linter issues detected and there is conflict in the locale files.

@@ -942,4 +942,10 @@ var migrations = []func(tx *sql.Tx) error{
_, err = tx.Exec(sql)
return err
},
func(tx *sql.Tx) (err error) {
sql := `ALTER TABLE users ADD COLUMN script text not null default '';`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it would not be better to rename this field to custom_js instead of script.

I know that there is an existing stylesheet field, but it would have been better to use custom_css.

<style nonce="{{ $stylesheetNonce }}">{{ .user.Stylesheet | safeCSS }}</style>
{{ end }}
{{ if .user.Script }}
<script type="module" nonce="{{ $stylesheetNonce }}">{{ .user.Script | safeJS }}</script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of reusing $stylesheetNonce, it would be preferable to rename this variable to something generic, for example: $cspNonce.

What is the benefit of using type="module" here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With type="module" you can use import and top-level await. In short, makes it easier to use modern JS features without a bundler, which is quite nice for something like this IMHO.

@milhnl
Copy link
Author

milhnl commented Sep 9, 2024

Rebased, linted, and fixed the variable names!

@@ -21,6 +21,7 @@ type User struct {
EntryDirection string `json:"entry_sorting_direction"`
EntryOrder string `json:"entry_sorting_order"`
Stylesheet string `json:"stylesheet"`
Script string `json:"script"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rename Script to CustomJS

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

<style nonce="{{ $stylesheetNonce }}">{{ .user.Stylesheet | safeCSS }}</style>
{{ if .user }}
{{ $cspNonce := nonce }}
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; img-src * data:; media-src *; frame-src *; style-src 'self'{{ if .user.Stylesheet }} 'nonce-{{ $cspNonce }}'{{ end }}{{ if .user.Script }}; script-src 'nonce-{{ $cspNonce }}'{{ end }}; require-trusted-types-for 'script'; trusted-types ttpolicy;">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSP is incorrect. The script-src directive should always includes self otherwise it won't load the application JS files.

it should be something like this: script-src 'self'{{ if .user.Script }} 'nonce-{{ $cspNonce }}'{{ end }}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that explains a regression I started noticing after the rebase. Fixed.

<style nonce="{{ $cspNonce }}">{{ .user.Stylesheet | safeCSS }}</style>
{{ end }}
{{ if .user.Script }}
<script type="module" nonce="{{ $cspNonce }}">{{ .user.Script | safeJS }}</script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@milhnl You forgot to rename .user.Script to .user.CustomJS in the template.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add user script option
2 participants